-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Re-implement Ammonite's Ctrl-C interruption for Scala REPL via bytecode instrumentation #24194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
8e6c29f to
ece60fa
Compare
a68d5b1 to
7fdf5c6
Compare
7bbaf9f to
6a15587
Compare
|
Might need some help with the failing tests. @bracevac do you have any idea? Maybe some classloader setup is different in those tests? |
|
The failing community build test has been fixed by now and should pass after a rebase. |
|
One option is to simply pass |
|
Got the tests green. The problem in that case was we were accidentslly instrumenting the clasloaders used in non-repl contexts as well. That has since been fixed |
|
One consequence of this is that REPL-loaded classes will be different from non-REPL-loaded classes, due to the bytecode instrumentation and class re-definition. So use cases embedding the REPL into an existing program to interact with it "live" would need to pass |
|
@lihaoyi could you clarify how you produced a REPL binary that works? Trying it with scala> def fix(x: Int): Int = fix(x)
1 warning found
-- Warning: --------------------------------------------------------------------
1 |def fix(x: Int): Int = fix(x)
|^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|Infinite recursive call
def fix(x: Int): Int
scala> fix(0)
^C
Interrupting running threadNothing happens at this point and a second |
compiler/src/dotty/tools/repl/ReplBytecodeInstrumentation.scala
Outdated
Show resolved
Hide resolved
compiler/src/dotty/tools/repl/ReplBytecodeInstrumentation.scala
Outdated
Show resolved
Hide resolved
| "-color:never", | ||
| "-Xrepl-disable-display" | ||
| "-Xrepl-disable-display", | ||
| "-Xrepl-disable-bytecode-instrumentation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing this option will indeed disable ^C interruption, but it will still show Interrupting running thread. Perhaps suppress the message fully?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Interrupting running thread is actually meant to indicate that Thread.interrupt is being called. The current logic does both by default, and only Thread.interrupt if instrumentation is disabled. Maybe we can change the message if it's confusing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, maybe a different message indicating that the option is active and it won't work. As it is, I find it confusing.
Co-authored-by: Oliver Bračevac <bracevac@users.noreply.github.com>
Co-authored-by: Oliver Bračevac <bracevac@users.noreply.github.com>
Co-authored-by: Oliver Bračevac <bracevac@users.noreply.github.com>
|
@bracevac the formatting is whatever the intellij default is. If there's an autoformatter I'll run it, but if there isn't an autoformatter I generally don't spend too much time fiddling with whitespace and just go with the defaults |
Co-authored-by: Oliver Bračevac <bracevac@users.noreply.github.com>
Since
Thread.stopwas removed (https://stackoverflow.com/questions/4426592/why-thread-stop-doesnt-work), the only way to re-implement such functionality is via bytecode instrumentation. This PR implements such functionality in the Scala REPL, such that we can nowCtrl-Cto interrupt runaway code that doesn't check forThread.isInterrupted()(which is what the currentThread.interruptdoes) without needing to kill the entire JVMThese snippets are now interruptable where they weren't before:
The way this works is that we instrument all bytecode that gets loaded into the REPL classloader using
scala.tools.asmand add checks at every backwards branch and start of each method body. These checks call a classloader-scopedReplCancel.stopCheckmethod, and theCtrl-Chandler is wired up to flip a var and make thestopCheck()calls fail.This adds some incremental performance hit to code running in the Scala REPL, but the result of no longer needing to trash your entire REPL process and session history due to a single runaway command is probably worth it. There may be other ways to instrument the code to minimize the performance hit. Some rough benchmarks:
if (boolean) throw(the current implementation): ~2ns per loop1/int, which throws whenint == 0: ~2ns per loopif (Thread.interrupted()): ~2ns per loopAn exponential-but-technically-not-infinite recursion benchmark below shows a minor slowdown from the start-of-method-body instrumentation (~6%):
This 50% slowdown is the worst case slowdown that instrumentation adds; anything more complex than a
while(true) x += 1loop will have a longer time taken, and the % slowdown from instrumentation would be smaller. Probably can expect a 10-20% slowdown on more typical codeThis instrumentation is on by default on the assumption that most REPL work isn't performance sensitive, but I added a flag to switch it off and fall back to the prior un-instrumented behavior which would require terminating the process to stop runaway code.
One consequence of this is that REPL-loaded classes will be different from non-REPL-loaded classes, due to the bytecode instrumentation and class re-definition. So use cases embedding the REPL into an existing program to interact with it "live" would need to pass
-Xrepl-disable-bytecode-instrumentationto allow classes and instances to be shared between themThe
jshellREPL also allows interruption of these snippets, and likely uses a similar approach though I haven't checked